tests/api/test_variant_lookup.py (2)

449-457: LGTM! Consider using a context manager for consistency.

This test function effectively checks the logging behavior of VariantOverlapDetector. The test correctly verifies that the expected debug message is logged when querying a non-existent region.

For consistency with the FileBasedVariantLookup test, consider using a context manager for VariantOverlapDetector if it supports it. This would ensure proper resource management and maintain a consistent style across similar tests.

---

Line range hint 480-507: LGTM! Good test design with randomization and proper resource management.

This test function effectively checks the logging behavior when a chromosome is missing from one of the VCFs. The use of a context manager for FileBasedVariantLookup ensures proper resource management. The shuffling of VCF paths is an excellent practice to ensure the test is not order-dependent and is more robust.

Regarding the existing comment about moving assertions outside the context manager:

with FileBasedVariantLookup(
    vcf_paths=vcf_paths, min_maf=0.00, include_missing_mafs=True
) as variant_lookup:
    variants_of_interest = variant_lookup.query(
        refname="chr2", start=7999, end=9900
    )
assert len(variants_of_interest) == 14
expected_error_msg = "does not contain chromosome"
assert expected_error_msg in caplog.text

The suggestion to move the assertions outside the context manager is valid and can be implemented. This change would not affect the test's functionality and might slightly improve readability. However, the current implementation is also correct and doesn't pose any issues.

Configuration used: CodeRabbit UI
Review profile: CHILL

prymer/api/variant_lookup.py (1)

325-328: Consider adding a comment for the empty close() method

The close() method is currently empty. If this is intentional (e.g., to be overridden by subclasses), consider adding a comment to explain this design choice.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 327-327: prymer/api/variant_lookup.py#L327
Added line #L327 was not covered by tests

Configuration used: CodeRabbit UI
Review profile: CHILL

tests/api/test_variant_lookup.py (2)

448-455: Consider consolidating duplicate test logic.

Both test functions share similar setup and assertions. Consider using a parameterized test to reduce code duplication.

-def test_simple_variant_conversion_logs_file_based(
-    vcf_path: Path, caplog: pytest.LogCaptureFixture
-) -> None:
-    caplog.set_level(logging.DEBUG)
-    with FileBasedVariantLookup(
-        vcf_paths=[vcf_path], min_maf=0.01, include_missing_mafs=False
-    ) as variant_lookup:
-        variant_lookup.query(refname="foo", start=1, end=2)
-        assert "No variants extracted from region of interest" in caplog.text
-
-def test_simple_variant_conversion_logs_non_file_based(
-    vcf_path: Path, caplog: pytest.LogCaptureFixture
-) -> None:
-    caplog.set_level(logging.DEBUG)
-    variant_lookup = VariantOverlapDetector(
-        vcf_paths=[vcf_path], min_maf=0.01, include_missing_mafs=False
-    )
-    variant_lookup.query(refname="foo", start=1, end=2)
-    assert "No variants extracted from region of interest" in caplog.text
+@pytest.mark.parametrize(
+    "lookup_class,context_manager",
+    [
+        (FileBasedVariantLookup, True),
+        (VariantOverlapDetector, False),
+    ],
+)
+def test_simple_variant_conversion_logs(
+    vcf_path: Path,
+    caplog: pytest.LogCaptureFixture,
+    lookup_class: Type,
+    context_manager: bool,
+) -> None:
+    caplog.set_level(logging.DEBUG)
+    kwargs = {"vcf_paths": [vcf_path], "min_maf": 0.01, "include_missing_mafs": False}
+    
+    if context_manager:
+        with lookup_class(**kwargs) as variant_lookup:
+            variant_lookup.query(refname="foo", start=1, end=2)
+    else:
+        variant_lookup = lookup_class(**kwargs)
+        variant_lookup.query(refname="foo", start=1, end=2)
+    
+    assert "No variants extracted from region of interest" in caplog.text

---

601-613: Improve code comment clarity.

The inline comment about "common-mixed-2/2" could be clearer about the VCF format specifics.

-                end=9100,  # while "common-mixed-2/2" starts at 9101, in the VCf is starts at 9100
+                end=9100,  # Note: In VCF format, "common-mixed-2/2" is part of a multi-allelic variant at position 9100

Configuration used: CodeRabbit UI
Review profile: CHILL

prymer/api/variant_lookup.py (1)

364-368: Remove redundant return None statement in __exit__ method

Explicitly returning None at the end of the __exit__ method is unnecessary because Python functions return None by default if no return statement is present.

Apply this diff to simplify the method:

 def __exit__(
     self,
     exc_type: Optional[type[BaseException]],
     exc_value: Optional[BaseException],
     traceback: Optional[TracebackType],
 ) -> None:
     """Exit this context manager while closing the underlying VCF handles."""
     self.close()
-    return None

Configuration used: CodeRabbit UI
Review profile: CHILL